Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: tweaking relative path handling for windows compatibility #18

Merged
merged 11 commits into from
Dec 14, 2023

Conversation

aorumbayev
Copy link
Contributor

@aorumbayev aorumbayev commented Dec 6, 2023

Proposed changes

  1. Fixing path resolution for windows paths in resolveFilePath method
  2. Adding shx to make compile script available on windows as well
  3. Adding tweaks to esbuild to target node since fix relies on path. @jasonpaulos given that its a vscode extension related core dependency - safe to assume we can target node? I think all electron apps got node available

@aorumbayev
Copy link
Contributor Author

@jasonpaulos would appreciate if you could trigger a new deployment to npm once this is reviewed and merged

Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can help figure out the best way to support windows paths in the debug adapter, but it will be much easier if we have reliable testing on windows. Since this is an area you've shown interest in, would you be willing to modify the github actions workflow to also build and test on windows?

package.json Outdated Show resolved Hide resolved
src/common/utils.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@aorumbayev
Copy link
Contributor Author

Thanks @jasonpaulos. I pushed a tweak to ci to include windows worker, will check the remaining comments tomorrow

package.json Show resolved Hide resolved
src/common/utils.ts Outdated Show resolved Hide resolved
@aorumbayev
Copy link
Contributor Author

aorumbayev commented Dec 7, 2023

@jasonpaulos i'm debugging the windows tests, majority of them fail given the tests using unix like path syntax and i assume they probably weren't tested on windows prior. Will let you know if ill hit blockers that might need your aid. I have a separate pipeline on forked repo where im testing it, you can assume the error on the current pr is already fixed (adding gitattributes for windows to us LS)

Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sounds good. I'll just leave these two comments with some insights from my end

.github/workflows/ci.yml Show resolved Hide resolved
src/common/utils.ts Outdated Show resolved Hide resolved
@aorumbayev
Copy link
Contributor Author

@jasonpaulos i pushed interim tweaks for eol. You will now observe the test failures, if some seem obvious to fix please let me know. I assumed its trivial tweak in path strings but seems like it requires digging a bit more into adapter implementation. Also seems like pipeline requires approval upon each trigger - any chance you can disable this for this pr or add myself and @neilcampbell to contributors on the repo?

@jasonpaulos
Copy link
Contributor

You should be able to run the workflow without my approval now

@jasonpaulos
Copy link
Contributor

Thanks, I see the test failures now. I'll see if I can diagnose the problem

* chore: wip testing windows compatibility

* chore: testing
@jasonpaulos
Copy link
Contributor

I hope you don't mind but I pushed some changes to this PR. I liked the idea of expanding FileAccessor to implement this function, but there were a few things I wanted to tweak. Please let me know if this looks good to you @aorumbayev and @neilcampbell.

Specifically, if you have access to a windows machine, can you please run the extension and ensure the FileAccessor for vscode is implemented properly? Since that relies on the vscode runtime package I unfortunately can't think of a way to automate that test.

@aorumbayev
Copy link
Contributor Author

@jasonpaulos awesome, thanks for pushing the tweaks. Smoke tested this on windows independently and as a dependency in our extension repo - works fine 👍

I can't self approve the repo so feel free to merge as time allows.

@jasonpaulos jasonpaulos merged commit 2528238 into algorand:main Dec 14, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants